notice when app engine java components aren't installed #207
Conversation
| @@ -262,11 +290,20 @@ public void validate() throws AppEngineException { | |||
| "Validation Error: Java Tools jar location '" | |||
| + JAR_LOCATIONS.get(JAVA_TOOLS_JAR) + "' is not a file."); | |||
| } | |||
| try { | |||
| if (!isComponentInstalled("app-engine-java")) { | |||
There was a problem hiding this comment.
Isn't the block above that checks the existence of JAVA_TOOLS_JAR sufficient to tell if the component is installed? I'm still not a fan of doing this expensive check using gcloud components list.
There was a problem hiding this comment.
👍 to @meltsufin's comment: I tried running gcloud from a VM shared drive and it was glacially sloooow. If JAVA_TOOLS_JAR (or one of the jars) isn't available then we're either dealing with a missing app-engine-java component, or appengine-plugins-core is out of sync with the installed version and
There was a problem hiding this comment.
Could we use JAVA_TOOLS_JAR as a heuristic: if the path exists, then we seem to have app-engine-java installed, otherwise confirm with gcloud components list.
(or better yet, …/.install/app-engine-java.manifest, since that's most specific.)
There was a problem hiding this comment.
I've just tried running gcloud components remove app-engine-java on my Linux machine and it did delete ~/google-cloud-sdk/platform/google_appengine/google/appengine/tools/java/lib/appengine-tools-api.jar. When installed it back using gcloud components install app-engine-java, it reappeared. So, I think checking for existence of JAVA_TOOLS_JAR is sufficient.
@elharo, do you see a different result when you run these commands?
There was a problem hiding this comment.
The practice of depending on the Cloud SDKs internal directory structure is not OK with the Cloud SDK team. They reserve the right to break this at their discretion. We need to move away from depending on incidental files distributed with the SDK and moving to using their api/cli for this check is part of that.
This is also why we're moving to using Maven central as the source for classpath deps.
There was a problem hiding this comment.
It's true that the Cloud SDK reserves the right to move the files, but until we drop the dependency on AppCfg in the app-engine-java component in the Cloud SDK, we can do the file-based check without making anything more fragile in the library.
There was a problem hiding this comment.
Staging is in the process of being distributed to Maven Central. We should
be extracting ourselves out of this mess, rather than digging deeper into
it.
On Thu, Sep 8, 2016 at 12:46 PM, meltsufin [email protected] wrote:
In src/main/java/com/google/cloud/tools/appengine/cloudsdk/CloudSdk.java
#207 (comment)
:@@ -262,11 +290,20 @@ public void validate() throws AppEngineException {
"Validation Error: Java Tools jar location '"
+ JAR_LOCATIONS.get(JAVA_TOOLS_JAR) + "' is not a file.");
}
- try {
if (!isComponentInstalled("app-engine-java")) {It's true that the Cloud SDK reserves the right to move the files, but
until we drop the dependency on AppCfg in the app-engine-java component in
the Cloud SDK, we can do the file-based check without making anything more
fragile in the library.—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/GoogleCloudPlatform/appengine-plugins-core/pull/207/files/29147b0c66ac9c154d990808acfebec1ffc8f716#r78044155,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AHf5HRitzbZ-DsADGWSLzsSPa2aR3TLxks5qoDvvgaJpZM4J35Q-
.
|
|
||
| private CloudSdk(Path sdkPath, | ||
| String appCommandMetricsEnvironment, | ||
| String appCommandMetricsEnvironmentVersion, | ||
| @Nullable File appCommandCredentialFile, | ||
| String appCommandOutputFormat, | ||
| ProcessRunner processRunner, | ||
| WaitingProcessOutputLineListener runDevAppServerWaitListener) { | ||
| WaitingProcessOutputLineListener outputLineListener, | ||
| AccumulatingLineListener stdOutput) { |
There was a problem hiding this comment.
Do we really need two kinds of listener API's? It seems unlikely the user would want to pass both?
There was a problem hiding this comment.
They do quite different things. It might make sense to move the AccumulatingLineListener into ProcessRunner itself. It's not exposed in the public API (constructor is private)
| @@ -93,6 +93,11 @@ | |||
| <artifactId>jsr305</artifactId> | |||
| <version>3.0.1</version> | |||
| </dependency> | |||
| <dependency> | |||
| <groupId>org.json</groupId> | |||
There was a problem hiding this comment.
I sort of half read the other json related thread you have going. Are we switching this to gson?
There was a problem hiding this comment.
I don't think so. GSON would be dramatic overkill for reading a single value. We could use JSR 353 if the license (GPL, CDDL) isn't a problem.
|
I'm not fully sure what was discussed here, but we're already depending on the |
|
This check though is for whether the java app engine component has been On Thu, Sep 8, 2016 at 6:13 PM, Appu Goundan [email protected]
|
|
When we move away from relying on the |
|
There appears to be something really weird going on with the backup gcloud creates on removing the app-engine-java component, at least on Mac OS X. Somehow my terminal gets redirected to look there instead of where it used to be. That explains why Mike thinks the tools jar is removed and I think it isn't. It's actually being moved from point A to point B, and perhaps a link left behind. |
|
@briandealwis After an offline discussion I've been convinced that checking the jar location is actually the best approach for now. Getting this behavior into the library is blocking our IJ release (since we want this feature for our next beta). Anybody have bandwidth to start on this today? |
@meltsufin @joaoandremartins @etanshaul @markpell
fix #165
Uses gcloud components list